-
Notifications
You must be signed in to change notification settings - Fork 9
feat(web): add web generator #285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
===========================================
+ Coverage 51.15% 63.30% +12.15%
===========================================
Files 81 128 +47
Lines 6956 10471 +3515
Branches 306 613 +307
===========================================
+ Hits 3558 6629 +3071
- Misses 3395 3838 +443
- Partials 3 4 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
const language = matches?.groups?.language ?? ''; | ||
const [copyText, setCopyText] = useState('Copy to clipboard'); | ||
|
||
const handleCopy = async text => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not including useCopyToClipord in ui lib ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC I didn't include it since it didn't fall under the "UI Component" category, it's a hook, however, if you disagree, I won't block.
"estree-util-value-to-estree": "^3.4.0", | ||
"estree-util-visit": "^2.0.0", | ||
"github-slugger": "^2.0.0", | ||
"glob": "^11.0.2", | ||
"hast-util-to-string": "^3.0.1", | ||
"hastscript": "^9.0.1", | ||
"html-minifier-terser": "^7.2.0", | ||
"mustache": "^4.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use __<VARIABLE>__
like in the other generators, however, the JavaScript I was trying to add to the HTML broke the structure of the file. Using a library like Mustache properly escapes anything injected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const TEMPLATE = ({...props}) => dedent`
<html>
${props.foo}
</html>
`;
this can work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I wasn’t clear, the compiled JavaScript broke the HTML structure because it contained characters that need to be escaped to be in an HTML script.
For example, in your scenario, if props.foo
contains a </html>
, it’ll escape the sandbox.
Mustache performs all the needed escaping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that we're using yet another layer of whatever freakery here. Mustache is great, but it feels like you're throing a nuke at an ant's sized problem. Not to mention adding yet another engine here will make things even more complex and costly-performance wise. Let's try to avoid throwing npm packages to any problem we encounter, shall we?
🎉 The code now dehydrates to the client so it can render without JavaScript! |
Wow 😵💫 and what about codetab |
It rehydrates and runs with JS, but if you don't have JS, you can still view the docs. I used React's SSRing |
@AugustinMauroy and I got search to finally work 🎉 |
024bbea
to
dbfe55d
Compare
@nodejs/nodejs-website @nodejs/web-infra |
I think it should be improved prior to merge. |
Around 1.3mb, with 1mb coming from Orama. The actual page content is ~96kb.
The position of the
Yes. The stability index component is
Looking at the Flamegraph, to generate everything (for me) takes 136s1, the "hottest" (longest) pieces were PostCSS (especially Tailwind) and minification, followed by Shiki. For reference, without minification, the process takes roughly 110s. @Harkunwar said he was going to work on compiling the
When generated without the
Would you prefer we hide the scrollbar, like we do on the right side?
I'm open to a new component. If you'd like, I can give you edit access to the Figma (Slack message me your Figma user-email) for designing.
If you'd like to, we most definitely can, but it is in a state where it's ready for review, so I wouldn't want the draft state to discourage thorough review.
It's the same on https://nodejs.org, so any changes on that front should be done in
Ditto, see
Fixed in the latest commit.
It's the same on https://nodejs.org. IIRC we had a light-mode design, but it's in the Figma graveyard.
In our Figma design, the word "Stability" is not included, however, would you prefer one of the designs below?
As I mentioned above, "Would you prefer we hide the scrollbar, like we do on the right side?"
That's an issue for
Ditto, see
I just tab-ed through the entire page, and didn't see a point where the entire body was highlighted.
Fixed the HTML language issue in the latest commit, see nodejs/nodejs.org#7890 for the contrast issue, other issues are inherited from Footnotes
|
Jesus christ. Can we make the bundle of orama stuff separated? With a React Lazy import? |
I was referring to the unstyled Markdown in the content of the AlertBox
Nice.
Let's mock something outside of Figma. We can open an issue and come up with ideas tbh. It's just the current rendering is definitely not great
Fair. Can we add a blocked label?
Are these the same paddings from the website? Or is it because we don't have any selector on non-mobile for navigation on website? 🤔 But I'd rather not hiding the scrollbar on the left-side. Maybe just adjusting the padding.
Fine with either designs, but I actually prefer the pill. Extra feedback:
|
Per Michele Riva on the progress of making Orama tree-shakable: "Yes we are actively working on it! We are testing the components before releasing them in the coming week. We’ll open a PR when they're ready" So, let's see if they have something for us in the near future (this/next week), and if not, move forward with lazy loading. |
I was more thinking of what I shared before. We can reconstruct what would be the method structure: methodName(arg1, arg1, arg3,...): ReturnType1 (Think the above as the method having overloads if any). These would be within a code snippet as if they were a type definition of sort (see the link to next.js docs I've attached); We do have the structure of the method which is generated by the JSON Generator (and I suppose also resides on the AST?) And then below this we would have a table with the argument names, description, etc... That IMO would look the cleanest. |
Ahh I didn't see the link reference, I understand now. Thanks! |
Yeah, I see 👀 , anyways I do feel that these are not alright, but I agree that then it is out of scope here 🙂👍
Looks great! Thanks! 😄
ok then again out of scope but ideally something to follow up on 🙂👍
The second one for sure for me! 😄
I can see that you've hidden the scrollbar... no that's not the right solution in my opinion, I think that the version picker should have a max-width so that it doesn't overflow its container (causing the horizontal scrollbar not to appear at all since it won't be needed)
Ok, sorry for all of these
Yes... I am not seeing that anymore... I 100% saw that though... I'll try to see if it happens only after certain conditions 😕
👍 |
@dario-piotrowicz for your |
Now there's an evident flickering on the search element (also on the theme toggle button) in dark mode when I refresh the page: simplescreenrecorder.mp4(PS: they flicker in light mode too but not as evidently) |
do you mind if I open self isolated issues? (as they might be easier to deal with?) or maybe an issue with sub-issues? |
Same difference, just figured catch all might be less work for you, but that's even better :-) |
Ok I'll open isolated issues there 🙂👍 |
It's actually the Orama component loading in (which moves the toggle). Once Orama fixes their components, it'll be partially statically dehydrated, filling in the space |
Sorry for not following the PR instructions earlier 🙂 Yes I did run the orama-db generation and that does enable search for me! 😃 |
I'm just concerned what happens when there is a lot of nesting, for example,
That's going to be quite a table, no? |
As a personal design preference, I quite like the expandable function signatures, I think it gives a really cool way of displaying the parameters. For events, I was thinking of it showing |
I mean sure, but any other design would also be quite the thing. I guess we could refine. For example if you have 2 variations we could have:
TABLE CONTAINING VARIATION ONE
TABLE CONTAINING VARIATION TWO? |
Okay, I'll program the table, and we'll see how it looks. |
@ovflowd (Note: This is a first draft to show what a table would look it more-or-less, i.e. |
Nice, I still think we need to add that snippet of method signature that I mentioned. This would allow us to rename the header section to be simply the method name or class name. It also helps on Search Indexing |
Yes, the draft was just for the table, but I plan to use |
Fixes #7.
This PR adds the web generator.
Tasks / Issues
P1 – Must Complete Before Merge
P2 – Must complete before migration
ChangeHistory
component doesn’t render Markdown correctly. (This an issue with JSX generator, not the web generator)P3 – Can Be Done in a Follow-up
Get a preview
Footnotes
Add things as they appear, or leave review comments. ↩ ↩2 ↩3